(Thrift Field Masker Pt2) Implement field masking for thrift by jrhee17 · Pull Request #6311 · line/armeria

jrhee17 · 2025-07-10T14:18:30Z

Motivation:

Following field maskers for annotated services at #6232 , this PR attempts to introduce FieldMasker support for thrift.

To ensure that deserialization is also supported natively using TProtocol, the design of thrift uses a variant of TProtocol which keeps track of the field of a specified instance.
Unmasking support is to be done in the ensuing PR for easier reviews.

Modifications:

Introduce RpcRequestSerializer, RpcResponseSerializer to serialize rpc requests
ThriftFieldInfo, ThriftFieldMaskerSelector, and ThriftFieldMaskerSelectorProvider are implemented to support the FieldMasker API.
TBaseSelectorCache, TBaseSerializer, MaskingTProtocol are implemented to actually apply field masking during serialization
Added the ability to specify the annotations_as_metadata option to the gradle scripts

Result:

Users can use the FieldMasker API for thrift services

github-actions · 2025-07-10T15:31:23Z

🔍 Build Scan® (commit: `67a0386`)

Job name	Status	Build Scan®
build-ubicloud-standard-16-jdk-8	✅	https://ge.armeria.dev/s/bbsh7e2qnzm66
build-ubicloud-standard-16-jdk-21-snapshot-blockhound	❌ (failure)	https://ge.armeria.dev/s/7vwureqegtg3y
build-ubicloud-standard-16-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/44s3xfobuhera
build-ubicloud-standard-16-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/gtaoa2d2vng2g
build-ubicloud-standard-16-jdk-17-leak	✅	https://ge.armeria.dev/s/ihoq7up7q4ot6
build-ubicloud-standard-16-jdk-11	✅	https://ge.armeria.dev/s/lcxj2xiboqoj2
build-macos-latest-jdk-21	❌ (failure)	https://ge.armeria.dev/s/iidz7n6t2piio

jrhee17 · 2025-07-16T01:19:36Z

ready for review

ikhoon · 2025-07-16T05:48:01Z

+
+    private static ContentSanitizer<String> commonContentSanitizer() {
+        return ContentSanitizer.builder()
+                               .fieldMaskerSelector(ThriftFieldMaskerSelector.of(info -> {


What do you think of adding a builder to create a ThriftFieldMaskerSelector in a declarative way for simple cases?

For example:

ThriftFieldMaskerSelector .buider() .onFieldAnnotation("secret", FieldMasker.nullify()) .onFieldAnnotation("grade", "red", new CustomFieldMasker()) ... .build();

Added a variant of ThriftFieldMaskerSelector to thrift0.19> since that's when annotations were supported.
Note that this the variant still requires that the thrift idl is compiled with the annotations_as_metadata option

It looks nice. Should we add a integration test case using ThriftFieldMaskerSelectorBuilder as an example so that users can reference it?

minwoox

Looks good all in all. 👍

minwoox

Looks good all in all. 👍

minwoox

Looks great. 👍

ikhoon

Thanks, @jrhee17! 🙇‍♂️🚀

ikhoon · 2025-07-23T13:58:37Z

+import com.linecorp.armeria.common.logging.FieldMasker;
+
+/**
+ * Holds information about a thrift struct field.


Suggested change

* Holds information about a thrift struct field.

* Holds information about a Thrift struct field.

ikhoon · 2025-07-24T06:46:27Z

+            @Override
+            public FieldMasker fieldMasker(ThriftFieldInfo info) {
+                for (AnnotationAndMasker rule : rules) {
+                    if (rule.matches(info.fieldMetaData().getFieldAnnotations())) {


Should we extract info.fieldMetaData().getFieldAnnotations() into a local variable since getFieldAnnotations() may create UnmodifiableMap for each invocation.

// Copied from the generated `SecretStruct` new org.apache.thrift.meta_data.FieldMetaData("secret", org.apache.thrift.TFieldRequirementType.DEFAULT, new org.apache.thrift.meta_data.FieldValueMetaData(org.apache.thrift.protocol.TType.STRING), java.util.stream.Stream.<java.util.Map.Entry<java.lang.String, java.lang.String>>builder() .add(new java.util.AbstractMap.SimpleImmutableEntry<>("grade", "red")) .build().collect(java.util.stream.Collectors.toMap(java.util.Map.Entry::getKey, java.util.Map.Entry::getValue))));

ikhoon · 2025-07-24T06:56:02Z

+
+    private static ContentSanitizer<String> commonContentSanitizer() {
+        return ContentSanitizer.builder()
+                               .fieldMaskerSelector(ThriftFieldMaskerSelector.of(info -> {


It looks nice. Should we add a integration test case using ThriftFieldMaskerSelectorBuilder as an example so that users can reference it?

codecov · 2025-07-28T05:54:47Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 0.00%. Comparing base (8150425) to head (67a0386).
⚠️ Report is 126 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #6311       +/-   ##
============================================
- Coverage     74.46%       0   -74.47%     
============================================
  Files          1963       0     -1963     
  Lines         82437       0    -82437     
  Branches      10764       0    -10764     
============================================
- Hits          61385       0    -61385     
+ Misses        15918       0    -15918     
+ Partials       5134       0     -5134

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Motivation: Following the implementation of #6311, this changeset attempts to implement deserialization of masked fields. Doing so would allow users to define a `FieldMasker` as follows: ``` FieldMasker.builder() .addMasker( NormalFooStruct.class, struct -> encryptToString(struct), str -> return decryptToStruct(str)); ``` Conceptually, this implementation is a corollary of `MaskingBeanDeserializerModifier`. There is no change in the public API as `FieldMaskerBuilder#addMasker(Class, Function, Function)` is already exposed. Modifications: - Added `UnMaskingTProtocol`, `TMaskingDeserializer`, `UnMaskingContexts` which helps with the implementation of unmasking - Deserialization requires instantiation of the default instance. To aid with this, `TBaseCache` has been introduced. Result: - The unmasking API functions correctly for thrift structs

jrhee17 added 3 commits July 10, 2025 10:27

remove redundant thrift files

7fa60bf

remove the compare script

b962556

implement field masking for thrift

6dba570

jrhee17 added this to the 1.33.0 milestone Jul 10, 2025

jrhee17 added the new feature label Jul 10, 2025

jrhee17 added 3 commits July 15, 2025 19:52

Merge branch 'main' into feat/thrift-log-maskers-tproto-part2

052e677

cleanup

7dfe95e

cleanup

0c2fc3b

jrhee17 marked this pull request as ready for review July 16, 2025 01:19

jrhee17 requested review from ikhoon, minwoox and trustin as code owners July 16, 2025 01:19

ikhoon reviewed Jul 16, 2025

View reviewed changes

jrhee17 added 2 commits July 17, 2025 19:49

address comments by @ikhoon

6feccb7

address comment by @ikhoon

e13279e

minwoox reviewed Jul 23, 2025

View reviewed changes

jrhee17 added 2 commits July 24, 2025 09:56

Merge branch 'main' into feat/thrift-log-maskers-tproto-part2

7daa41f

address comments by @minwoox

41ade08

minwoox approved these changes Jul 24, 2025

View reviewed changes

ikhoon approved these changes Jul 24, 2025

View reviewed changes

address comments by @ikhoon

67a0386

jrhee17 merged commit 883e8bf into line:main Jul 28, 2025
11 of 14 checks passed

jrhee17 mentioned this pull request Jul 29, 2025

Implement unmasking for Thrift FieldMasker #6330

Merged

	* Holds information about a thrift struct field.
	* Holds information about a Thrift struct field.

Uh oh!

Conversation

jrhee17 commented Jul 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jul 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔍 Build Scan® (commit: 67a0386)

Uh oh!

jrhee17 commented Jul 16, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ikhoon Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

jrhee17 Jul 18, 2025

Choose a reason for hiding this comment

Uh oh!

ikhoon Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

minwoox left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

minwoox left a comment

Choose a reason for hiding this comment

Uh oh!

minwoox left a comment

Choose a reason for hiding this comment

Uh oh!

ikhoon left a comment

Choose a reason for hiding this comment

Uh oh!

ikhoon Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

ikhoon Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

ikhoon Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov Bot commented Jul 28, 2025

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jrhee17 commented Jul 10, 2025 •

edited

Loading

github-actions Bot commented Jul 10, 2025 •

edited

Loading

🔍 Build Scan® (commit: `67a0386`)